Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix StackOverflowError on types with self-references #261

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Fix StackOverflowError on types with self-references #261

wants to merge 1 commit into from

Conversation

psteiger
Copy link

@psteiger psteiger commented Feb 14, 2024 •
edited
Loading

Description:

When we have self types (implemented by recursive generics), Motif fails with StackOverflowError:

	at com.uber.xprocessing.ext.XTypeKt.hasCollectionType(XType.kt:316)
	at com.uber.xprocessing.ext.XTypeKt.hasCollectionType(XType.kt:316)
	at com.uber.xprocessing.ext.XTypeKt.hasCollectionType(XType.kt:316)
        ...

The following code seems to trigger that condition:

open class BaseRouter<out R : BaseRouter<R, *>, I : BaseInteractor<*, *, *>>(
    interactor: I,
) : Router<I>(interactor)

abstract class BaseInteractor<P : Any, I : BaseInteractor<*, I, *>, R : BaseRouter<R, I>>(
    presenter: P,
) : Interactor<P, R>(presenter) 
public class ConcreteInteractor extends 
    BaseInteractor<EmptyPresenter, ConcreteInteractor, ConcreteRouter>

public class ConcreteRouter extends
    BaseRouter<ConcreteRouter, ConcreteInteractor>
   ...

This PR changes XType.hasCollectionType() implementation to:

  1. Not re-process types that were already processed
  2. Use a queue instead of recursion (to avoid StackOverflowError for big enough type trees).

Test

Created a local build, built and ran one of our apps with the local build.

@psteiger psteiger force-pushed the fix-infinite-recursion branch from 304ff4d to 13ad53a Compare February 14, 2024 16:13
@psteiger psteiger mentioned this pull request Feb 14, 2024
Open
@davissuber
Copy link
Contributor

Code looks great; could we add a test?

@jbarr21 jbarr21 self-requested a review February 15, 2024 00:42
@davissuber
Copy link
Contributor

davissuber commented Feb 15, 2024 •
edited
Loading

Some extra investigation notes regarding the room-compiler-processing library:

  • the version are on 2.6.0-alpha02 is closer to the latest version 2.6.1 than I originally thought
  • there aren't that much difference in the XType neighborhood between the two versions (based on 5-min of my untrained eyeballing), so bumping the library alone may not solve the stack overflow problem
  • however, based on this comment, it seems that hasCollectionType is introduced in part to handle some KSP type equality checks, which even the current version of room-compiler-processing has some special override
    • in other words, maybe we can circumvent the stack overflow problem by changing how we use the room-compiler-processing library (hand-wavy comments made without much context, and possibly a rather involved task)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tyvsmith tyvsmith tyvsmith left review comments

@jbarr21 jbarr21 jbarr21 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@psteiger @davissuber @tyvsmith @jbarr21